Skip to content

[go_router_builder] Add go_router StatefulShellRoute support to go_router_builder #4238

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 34 commits into from
Aug 2, 2023

Conversation

hannah-hyj
Copy link
Member

@hannah-hyj hannah-hyj commented Jun 16, 2023

This reverts commit 68f99cb4ac66ca445b37bdc8619875f163b739f3.
auto-submit bot pushed a commit that referenced this pull request Jul 7, 2023
@hannah-hyj hannah-hyj changed the title [draft][go_router_builder] Add go_router StatefulShellRoute support to go_router_builder [go_router_builder] Add go_router StatefulShellRoute support to go_router_builder Jul 10, 2023
@hannah-hyj hannah-hyj marked this pull request as ready for review July 10, 2023 01:03
@hannah-hyj hannah-hyj requested a review from chunhtai as a code owner July 10, 2023 01:03
required this.navigatorKey,
required super.routeDataClass,
required super.parent,
required super.parentNavigatorKey,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

StatefulShellBranch doesn't support parent navigator key

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should have another more generic base class for RouteBaseConfig so that it can be extended by StatefulShellBranchConfig without the unused logic or property

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to confirm, do you mean like

RouteOrBranchBaseConfig -> RouteBaseConfig -> (GoRouteConfig, StatefulShellRouteConfig, etc)
RouteOrBranchBaseConfig -> StatefulShellBranchConfig


@override
String get routeConstructorParameters =>
navigatorKey == null ? '' : 'navigatorKey: $navigatorKey,';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no navigator key, but we need to have a way to pass in
navigatorContainerBuilder and restorationScopeId

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

),
);
break;
default:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not use case 'TypedGoRoute':?
it should throw if it is anything else.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

value._children.addAll(reader.read('routes').listValue.map<RouteBaseConfig>(
(DartObject e) => RouteBaseConfig._fromAnnotation(
value._children.addAll(reader
.read(typeName == 'TypedStatefulShellRoute' ? 'branches' : 'routes')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can this be a getter in the RouteBaseConfig? In general we should try to have less check like this in this method as possible.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -493,11 +577,21 @@ RouteBase get $_routeGetterName => ${_invokesRouteConstructor()};
final String routesBit = _children.isEmpty
? ''
: '''
routes: [${_children.map((RouteBaseConfig e) => '${e._invokesRouteConstructor()},').join()}],
${routeDataClassName == 'StatefulShellRouteData' ? 'branches' : 'routes'}: [${_children.map((RouteBaseConfig e) => '${e._invokesRouteConstructor()},').join()}],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

''';
final String parentNavigatorKeyParameter = parentNavigatorKey == null
? ''
: 'parentNavigatorKey: $parentNavigatorKey,';

if (routeDataClassName == 'StatefulShellBranchData') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

class StatefulShellRouteConfig extends RouteBaseConfig {
StatefulShellRouteConfig._({
required super.routeDataClass,
required super.parent,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parent navigator key?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

parentNavigatorKey: _generateParameterGetterCode(
classElement,
parameterName: r'$parentNavigatorKey',
),
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add a default and throws? it will be useful to remind people when they add a new typed route

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

/// The parent navigator key string that is used for initialize the
/// `RouteBase` class this config generates.
final String? parentNavigatorKey;
static String _generateChildrenGetterName(String name) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably exposed as a getter for subclass to override.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's used in the factory function factory RouteBaseConfig._fromAnnotation and Instance members can’t be accessed from a factory constructor.

@hannah-hyj hannah-hyj requested a review from chunhtai July 31, 2023 21:40
Copy link
Contributor

@chunhtai chunhtai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, nice work!

@hannah-hyj hannah-hyj added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 1, 2023
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Aug 2, 2023
@auto-submit
Copy link
Contributor

auto-submit bot commented Aug 2, 2023

auto label is removed for flutter/packages, pr: 4238, due to Pull request flutter/packages/4238 is not in a mergeable state.

@hannah-hyj hannah-hyj added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 2, 2023
@auto-submit auto-submit bot merged commit c3a5fb9 into flutter:main Aug 2, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 3, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 3, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 3, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Aug 3, 2023
flutter/packages@4e4961a...d00c1f9

2023-08-03 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 2.21.1 to 2.21.2 (flutter/packages#4637)
2023-08-03 [email protected] Roll Flutter from b3f99ff to c00d241 (12 revisions) (flutter/packages#4640)
2023-08-03 [email protected] [path_provider] Add getApplicationCachePath() - implementations (flutter/packages#4619)
2023-08-03 [email protected] [various] Removed references to deprecated `TestWindow` APIs (flutter/packages#4558)
2023-08-02 49699333+dependabot[bot]@users.noreply.github.com Bump actions/labeler from 4.1.0 to 4.3.0 (flutter/packages#4432)
2023-08-02 [email protected] [go_router_builder]  Add go_router StatefulShellRoute support to go_router_builder (flutter/packages#4238)
2023-08-02 [email protected] [Tooling] Add google owned cache for dependencies as an option in ci (flutter/packages#4567)
2023-08-02 [email protected] Roll Flutter from 1d59196 to b3f99ff (32 revisions) (flutter/packages#4634)
2023-08-02 [email protected] [go_router_builder] Support `ShellRouteData` without `const` constructor  (flutter/packages#4627)
2023-08-02 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 2.21.0 to 2.21.1 (flutter/packages#4573)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages-flutter-autoroll
Please CC [email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
koji-1009

This comment was marked as off-topic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App p: go_router_builder
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[go_router_builder] Add go_router StatefulShellRoute support to go_router_builder
3 participants